daemon: add SdNotifyMonotonicUsec helper function
#435
Conversation
corhere
force-pushed
the
monotonic_usec
branch
from
c77b716 to
42c0168
Compare
|
@cgwalters ptal |
daemon/sdnotify.go
Outdated
| "os" | ||
| "strconv" | ||
|
|
||
| "golang.org/x/sys/unix" |
There was a problem hiding this comment.
I think this should go to sdnotify_unix.go.
Currently, as of main branch, all code is buildable on Window (even if it's just a stub). With this golang.org/x/sys/unix import, it no longer builds on Windows.
corhere
force-pushed
the
monotonic_usec
branch
2 times, most recently
from
42b71d7 to
bdf9303
Compare
I tried that at first. Unfortunately golang.org/x/sys/[email protected] is incompatible with Go 1.12. I carefully chose the oldest x/sys commit which provided the necessary syscalls and constants so that the module will continue to build on Go 1.12, as required by CI. Is there a problem with using an untagged commit as the required version? The MVS algorithm should consider tagged versions as higher precedence since their commit dates are all newer, AIUI, so it should not cause any dependency-hell grief in importing modules.
What should I put instead? |
We'll just switch to modern Go instead, I don't see much value in trying to support ancient versions.
I dunno, but will ask around. |
The synchronized service reload protocol added in systemd version 253
requires that the service provides a MONOTONIC_USEC field alongside the
RELOADING=1 notification message for synchronization purposes. The value
carried in this field must be the system CLOCK_MONOTONIC timestamp at
the time the notification message was generated as systemd compares it
to other CLOCK_MONOTONIC timestamps taken by pid1.
While the Go standard library does utilize CLOCK_MONOTONIC in the
implementation of package "time", the absolute monotonic timestamps in
time.Time values are not made available to programmers. Users familiar
with idiomatic usage of monotonic timestamps in Go might (incorrectly)
try to implement MONOTONIC_USEC using process-relative monotonic
timestamps, like so:
var processStart = time.Now()
func NotifyReloadingINCORRECT() {
ts := time.Since(processStart)/time.Microsecond // WRONG
msg := fmt.Sprintf(
daemon.SdNotifyReload+"\nMONOTONIC_USEC=%d", ts,
)
_, _ = daemon.SdNotify(false, msg)
}
Help users fall into the pit of success by providing a helper function
SdNotifyMonotonicUsec() which returns a MONOTONIC_USEC string which
encodes the system CLOCK_MONOTONIC timestamp in decimal microseconds, as
systemd expects.
Signed-off-by: Cory Snider <[email protected]>
corhere
force-pushed
the
monotonic_usec
branch
from
bdf9303 to
6d96518
Compare
| require github.com/godbus/dbus/v5 v5.1.0 | ||
| require ( | ||
| github.com/godbus/dbus/v5 v5.1.0 | ||
| golang.org/x/sys v0.1.0 |
There was a problem hiding this comment.
should we use something more recent now that we support go 1.23 as minimum version?
There was a problem hiding this comment.
Modules are expected to declare requirements on the minimum version of dependencies. This module is compatible with golang.org/x/sys v0.1.0 or any later version. Declaring a dependency on a newer version than the minimum compatible only serves to force unnecessary dependency bumps on modules that require go-systemd.
There was a problem hiding this comment.
Well yes but also not really though, because that is also the exact version used by this module. So if there is a CVE in golang.org/x/sys then we would not use the newer version for testing. And any module importing go-systemd and not using golang.org/x/sys themselves directly or via other deps would also end up on the older version.
Does it matter right now? Properly not, the only CVE I see for golang.org/x/sys on https://pkg.go.dev/vuln/list was before 0.1.0 so this here is most likely fine. But it also means we miss out on potential other bug fixes in the meantime.
And if we already require go 1.23 then there seems little reason to keep it on such an old version.
There was a problem hiding this comment.
But that's out of scope for this module; the only CVE this module would care about is a CVE in unix.ClockGettime as that's the only code it depends on.
There was a problem hiding this comment.
Sorry, I was a bit terse in my previous comment; basically for this;
And any module importing go-systemd and not using
golang.org/x/systhemselves directly or via other deps would also end up on the older version.
The MVS design is for each module to specify the lowest possible version of dependencies that's required to use the module. "lowest possible" can account for CVEs in the parts of the dependency; for this module that means; if there's a CVE or bug in unix.ClockGettime that impacts this module, then it can update the minimum required version accordingly.
But it's NOT this module's responsibility to "patch" other modules or codebases that happen to use this module as part of their codebase. If their code, or another module uses other parts of golang.org/x/sys that is impacted by a bug or CVE in golang.org/x/sys, then either the module itself, or the dependency that introduces golang.org/x/sys as a dependency can update the version.
Pre-emptively updating the version only causes code-churn for all consumers of this module; in this case even though this module only uses a single function from golang.org/x/sys, but would be enforcing all users to accept updating all other parts that may be used by them.
The synchronized service reload protocol added in systemd version 253 requires that the service provides a
MONOTONIC_USECfield alongside theRELOADING=1notification message for synchronization purposes. The value carried in this field must be the systemCLOCK_MONOTONICtimestamp at the time the notification message was generated as systemd compares it to otherCLOCK_MONOTONICtimestamps taken by pid1.While the Go standard library does utilize
CLOCK_MONOTONICin the implementation of package "time", the absolute monotonic timestamps intime.Timevalues are not made available to programmers. Users familiar with idiomatic usage of monotonic timestamps in Go might (incorrectly) try to implementMONOTONIC_USECusing process-relative monotonic timestamps, like so:Help users fall into the pit of success by providing a helper function
SdNotifyMonotonicUsec()which returns aMONOTONIC_USECvariable-assignment string which encodes the systemCLOCK_MONOTONICtimestamp in decimal microseconds, as systemd expects.